Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: include base path in path to start script #8651

Merged
merged 4 commits into from
Jan 24, 2023
Merged

Conversation

benmccann
Copy link
Member

fixes #2958

@changeset-bot
Copy link

changeset-bot bot commented Jan 21, 2023

🦋 Changeset detected

Latest commit: f5d0ac6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann
Copy link
Member Author

wtf. I hate turbo so much

Screenshot from 2023-01-21 09-41-00

@Rich-Harris
Copy link
Member

Is there a test we can add for this?

@benmccann
Copy link
Member Author

Is there a test we can add for this?

This one is tricky to add a test for because it will pretty much work as it is today. The only reason it's broken in the linked issue is because the users are accessing the site through a proxy, but I don't want to setup a proxy just for the test. We could write a test that looks for the string import { start } from "/path-base in the rendered source, but it feels a bit brittle. Would you like me to add that?

@benmccann benmccann added the paths.base bugs relating to `config.kit.paths.base` label Jan 23, 2023
@Rich-Harris
Copy link
Member

Nah, that's probably not a good test. The reason I think we ought to have a test is that it honestly feels like a Vite bug that /@fs/... and /base/@fs/... work interchangeably, and it would be good to be able to guard against changes one way or the other. Unless that's by design?

@Rich-Harris
Copy link
Member

(I feel like /@fs/... should probably 404 if base is configured...)

@benmccann
Copy link
Member Author

I agree with that. I think the test would be best as part of a PR to change the behavior in Vite. It'd be hard to change the behavior at the moment as users might be relying on it via their Vite plugins and it'd be a breaking change. This is highly related to vitejs/vite#7363 which I just linked to in the code comment in #8683. It's probably my top request for Vite 5 so with any luck I'll get to fixing it then now that I cleaned up all the other base path stuff in Vite 4

@benmccann
Copy link
Member Author

I added a comment so that no one unknowingly breaks things by changing the code because they're unaware the base is needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
paths.base bugs relating to `config.kit.paths.base`
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dev server does not apply base path correctly
2 participants